-
-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VOL refactor and cleanup #4856
base: develop
Are you sure you want to change the base?
VOL refactor and cleanup #4856
Conversation
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
So they don't sound like they are operating on connectors. Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Also, small code tidying Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Also simplify and enforce some modularity boundaries around them, so that other packages can't modify VOL data structures. Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
!! | ||
!! See C API: @ref H5VLcmp_connector_cls() | ||
!! | ||
SUBROUTINE H5VLcmp_connector_cls_f(are_same, conn_id1, conn_id2, hdferr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This routine is necessary because it's possible to have VOL connector IDs that are different, but point to the same underlying VOL connector class.
|
||
f_ptr = C_NULL_PTR | ||
CALL H5Pset_vol_f(fapl_id, vol_id, error, f_ptr) | ||
CALL check("H5Pset_vol_f",error,total_error) | ||
|
||
CALL H5Pget_vol_id_f(fapl_id, vol_id_out, error) | ||
CALL check("H5Pget_vol_id_f",error,total_error) | ||
CALL VERIFY("H5Pget_vol_id_f", vol_id_out, vol_id, total_error) | ||
are_same = .FALSE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use new routine instead of directly comparing IDs.
/** | ||
* @ingroup JH5VL | ||
* | ||
* H5VLcmp_connector_cls Determines whether two connector identifiers refer to the same connector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This routine is necessary because it's possible to have VOL connector IDs that are different, but point to the same underlying VOL connector class.
@@ -98,7 +98,8 @@ public void testH5VLget_connector_id() | |||
*/ | |||
String connector = System.getenv("HDF5_VOL_CONNECTOR"); | |||
if (connector == null) | |||
assertEquals(HDF5Constants.H5VL_NATIVE, native_id); | |||
assertTrue("H5.H5VLcmp_connector_cls(H5VL_NATIVE_NAME, native_id)", | |||
H5.H5VLcmp_connector_cls(HDF5Constants.H5VL_NATIVE, native_id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use new routine instead of directly comparing IDs.
@@ -125,7 +125,7 @@ H5A__create_common(H5VL_object_t *vol_obj, H5VL_loc_params_t *loc_params, const | |||
HGOTO_ERROR(H5E_ATTR, H5E_CANTINIT, H5I_INVALID_HID, "unable to create attribute"); | |||
|
|||
/* Register the new attribute and get an ID for it */ | |||
if ((ret_value = H5VL_register(H5I_ATTR, attr, vol_obj->connector, true)) < 0) | |||
if ((ret_value = H5VL_register(H5I_ATTR, attr, H5VL_OBJ_CONNECTOR(vol_obj), true)) < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes, and similar ones in non-H5VL packages, are to remove the code coupling to the H5VL package, allowing it to change implementation of H5VL data structures without rippling through the library.
@@ -931,33 +931,27 @@ H5CX_retrieve_state(H5CX_state_t **api_state) | |||
} /* end if */ | |||
|
|||
/* Keep a copy of the VOL connector property, if there is one */ | |||
if ((*head)->ctx.vol_connector_prop_valid && (*head)->ctx.vol_connector_prop.connector_id > 0) { | |||
if ((*head)->ctx.vol_connector_prop_valid && (*head)->ctx.vol_connector_prop.connector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VOL connector property stores a pointer to the connector struct now, instead of nesting an ID inside the property list.
@@ -88,12 +88,6 @@ static herr_t H5F__flush_api_common(hid_t object_id, H5F_scope_t scope, void **t | |||
/* Local Variables */ | |||
/*******************/ | |||
|
|||
/* Declare a free list to manage the H5VL_t struct */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused (and should not be used outside the H5VL package)
@@ -25,9 +25,6 @@ | |||
/* Public Macros */ | |||
/*****************/ | |||
|
|||
/* Identifier for the native VOL connector */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move private pieces to private header
@@ -36,37 +36,51 @@ | |||
/* Package Private Typedefs */ | |||
/****************************/ | |||
|
|||
/* Internal struct to track VOL connectors */ | |||
struct H5VL_connector_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One connector object per VOL connector class registered with the library
@@ -27,28 +27,33 @@ | |||
/* Library Private Macros */ | |||
/**************************/ | |||
|
|||
/* If the module using this macro is allowed access to the private variables, access them directly */ | |||
#ifdef H5VL_MODULE | |||
#define H5VL_OBJ_RC(VOL_OBJ) ((VOL_OBJ)->rc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getters, for the rest of the library packages. (Similar idiom as in H5Fprivate.h)
@@ -395,7 +396,12 @@ test_es_get_requests(void) | |||
TEST_ERROR; | |||
if (count != 1) | |||
TEST_ERROR; | |||
if (connector_ids[0] != connector_ids_g[0]) | |||
cmp_value = 0; | |||
if (H5VLcmp_connector_cls(&cmp_value, connector_ids[0], connector_ids_g[0]) < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use new comparison routine in tests.
HGOTO_ERROR(H5E_LINK, H5E_CANTCREATE, FAIL, "unable to create hard link"); | ||
|
||
/* Set the connector to use for async operations */ | ||
if (connector) | ||
*connector = (link_vol_obj ? H5VL_OBJ_CONNECTOR(link_vol_obj) : H5VL_OBJ_CONNECTOR(curr_vol_obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this returns a pointer to the connector, I think it should increment the ref count and require the caller to release it after it's done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strictly local within the source module and only used in one spot, so I don't think that is necessary here.
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a VOL connector ID"); | ||
|
||
/* Bypass the H5VLint layer, calling the VOL callback directly */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the original reason to bypass the H5VLint layer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not certain 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to have it be consistent, in the long term
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
@@ -671,12 +671,8 @@ h5tools_set_fapl_vol(hid_t fapl_id, h5tools_vol_info_t *vol_info) | |||
/* Check for VOL connectors that ship with the library, then try | |||
* registering by name if that fails. | |||
*/ | |||
if (!strcmp(vol_info->u.name, H5VL_NATIVE_NAME)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it would cause problems when trying to use h5tools with the native VOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tools use the native connector by default; no command-line option is necessary.
Cleanup and prepare for thread-safety changes.
Big ideas:
Small things: